Skip to content

fix: fail e2e scenarios when a required node dies - #416

Merged
matthias-wright merged 3 commits into
audit-may-2026from
m/e2e-tests
Jun 29, 2026
Merged

matthias-wright merged 3 commits into
audit-may-2026from
m/e2e-tests

Conversation

@matthias-wright

@matthias-wright matthias-wright commented Jun 17, 2026 •

Copy link
Copy Markdown
Collaborator

Addresses #318.

Changes:

  • node/args.rs: run_node_local now returns Handle<anyhow::Result<()>> and run_node_local_inner returns the supervise_node_tasks outcome instead of logging-and-swallowing it. Still no process::exit there (would orphan the child reth processes) — the caller decides.
  • e2e scenario binaries (stake_and_checkpoint, stake_and_join_with_outdated_ckpt, withdraw_and_exit, sync_from_genesis, observer, verify_consensus_state_proof, protocol_params): each node spawn closure now fails the scenario (eprintln + exit(1)) when run_node_local returns Err. Safe against the intentionally-exiting withdrawn/stopped nodes, which take a coordinated-shutdown path and return Ok. Required making the spawn closures move (the message captures the Copy node index).
  • node/bin/testnet.rs: try_join_all now inspects each node's inner Result and logs failures.

@sebastian-osec

Copy link
Copy Markdown

I think it is not completely fixed yet.

Some of the predicates are still partial. For example, stake_and_checkpoint and stake_and_join_with_outdated_ckpt still skip node0 in the final progress loop, and withdraw_and_exit / sync_from_genesis still check progress only for 0..(NUM_NODES - 1). That may be intentional for stopped/withdrawn nodes, but the scenario still does not explicitly assert the expected shutdown/removal outcome for every omitted participant.

There are also still join paths that only print thread join failures and then continue to “All nodes shut down cleanly” / exit(0), for example in observer and the checkpoint scenarios. testnet.rs similarly logs inner node errors from try_join_all but does not fail the process.

Each scenario should define the required participant set and either assert progress for each required node or assert the expected clean shutdown/removal for intentionally stopped/withdrawn nodes, and join failures should become nonzero scenario failures.

@matthias-wright

Copy link
Copy Markdown
Collaborator Author

Update(b4507b1):

  • Fail e2e scenarios on unexpected node-handle exit or join failure

@sebastian-osec

Copy link
Copy Markdown

Thanks.

I think there is one remaining similar path in protocol_params.rs: the node wrapper still treats _ = node_handle.fuse() as just println!("Node {} handle completed", x). Since there is no expected withdrawn/stopped participant in that scenario, an early handle completion should probably be treated the same way as the other scenarios and fail the scenario.

So I think the update fixes my comment for the touched paths, but I would still patch protocol_params.rs before closing #318 completely.

@matthias-wright

Copy link
Copy Markdown
Collaborator Author

Update(018839f):

  • Fail e2e scenarios on remaining unguarded node-exit paths

@sebastian-osec

Copy link
Copy Markdown

This looks good to me.

@matthias-wright
matthias-wright merged commit a87aaa0 into audit-may-2026 Jun 29, 2026
7 of 8 checks passed
@matthias-wright
matthias-wright deleted the m/e2e-tests branch June 29, 2026 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants